Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(upgrade): introduce tracing as an optional unstable feature #3326

Merged
merged 16 commits into from
Oct 2, 2023

Conversation

RamziA961
Copy link
Contributor

This change allow users to opt out of tracing via the tracing crate by adding tracing as an optional feature. This change is part of the effort, outlined in #2874, to reach hyper 1.0.

Closes #3319
BREAKING CHANGES: tracing is disabled by default and requires users to opt in to revert to previous behavior.

@RamziA961 RamziA961 force-pushed the master branch 2 times, most recently from 57c5827 to 25f10dc Compare September 27, 2023 14:00
This change allow users to opt out of tracing via the `tracing` crate
by adding tracing as an optional feature. This change is part of the
effort, outlined in hyperium#2874, to reach hyper 1.0.

Closes hyperium#3319
BREAKING CHANGES: tracing is disabled by default and requires users to
opt in to revert to previous behavior.
@seanmonstar
Copy link
Member

Thanks for taking this on!

We need to make it unstable similar to the FFI feature, with a check in lib.rs that a cfg(hyper_unstable_tracing) was also passed, or trigger a compiler error. And then document it in the unstable features section of the same file.

Separately, one possible way I think this could be done is making an internal module with trace and debug macros, and inside those out the cfg checks. It's not a big deal, it just might be easier.

@RamziA961
Copy link
Contributor Author

Thanks for the feedback. An internal module would definitely be a cleaner approach, and would help with unused variable warnings. I'll get started on that.

Introduce several macros that act as wrappers around the `tracing`
module's macros to allow for more concise syntax for conditional
compilation of `tracing` macro calls. As `tracing` is unstable,
the new `trace` module will facilitate transitioning `tracing`
to an optional feature, as outlined in hyperium#2874 and hyperium#3326.

BREAKING CHANGES: Integration of macros requires the replacement of
uses of `trace` macros.
…al trace module

Refactor code to utilize tracing macros from the new `trace` module and
fix breaking changes described in commit aba2a35. Part of the effort to
transition `tracing` to an unstable feature hyperium#3326 #hyperium#2874.
…ompilation.

Usage of variables passed into the `trace` module is dependent on
whether the `tracing` feature is enabled, thus producing a compiler
warning when `tracing` is disabled. Prefix potentially unused variables
with an underscore to indicate that this is intentional.
Add a `rustc` configuration flag (`hyper_unstable_tracing`) that must be passed
to the compiler when using the `tracing` feature. Throw a compiler error if `tracing`
is enabled without the flag. Part of the effort to transtition `tracing` to an
unstable dependecy. See hyperium#3326 and hyperium#2874.
…sage

Add curly braces around a call to the `trace::debug` macro within a
`map_err` anonymous function. The change is necessary to convert the
function from an expression to a block to allow for conditional
compilation.
…ature list

Document `trace` and add new features to the documentation feature list.
Modify `package.metadata.docs.rs` in `Cargo.toml` to include `tracing`
to list of `features` and `--cfg hyper_unstable_tracing` to the list of
`rustdoc-args`.
@RamziA961
Copy link
Contributor Author

RamziA961 commented Sep 28, 2023

I think a quick overview of the changes I made might be helpful in reviewing the PR, so here goes:

  • I introduced a module called trace, which essentially exposes macros that act as wrappers around tracing's macros but handle conditional compilation internally. The caveat is tracing has span macros and they aren't so easily replaceable. Their uses throughout the codebase are sparse and all uses comprised of creating a Span and then calling the method enter(). Both the Span and Entered instances need to be bound to a variable so that they are not dropped. The only approach that I could think of the preserves the conciseness of the original syntax is the following but it is greatly restrictive:
macro_rules! debug_span {
    ($($arg:tt)*) => {
        #[cfg(feature = "tracing")]
        {
            #[cfg(not(hyper_unstable_tracing))]
            compile_error!(
                "\
                The `tracing` feature is unstable, and requires the \
                `RUSTFLAGS='--cfg hyper_unstable_tracing'` environment variable to be set.\
            "
            );
            let span = tracing::debug_span!($($arg)+);
            let _ = span.enter();
        }
    }
}
  • I replaced uses of tracing's macros with the new internal trace module's macros. In most cases, it simply required deleting use tracing::{x, y, z}, except when the macros were used as part of an expression, e.g., something.map_err(|e| tracing::debug!("{e:?}")).

  • Some variables had to be prefixed with an underscore since unused variables warnings are emitted when compiling with tracing disabled.

  • To ensure that the code compiles when the tracing feature is off, the trace module must always be declared in lib so that the compiler can expand invocations of the macros into nothing.

  • I introduced a new feature tracing and a new cfg flag --hyper_unstable_tracing. The feature must be enabled and the flag must be passed to rustc for tracing to be enabled, otherwise a compiler error is thrown in similar fashion to ffi. This is the cause of the failing CI test.

  • Documentation for the module, feature, and cfg flag are provided.

I think that covers it. Please let me know what you think and if there are any improvements I can make. Thanks!

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much, the work is very thorough, and your write up is spectacular.

src/lib.rs Outdated Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
src/proto/h2/client.rs Outdated Show resolved Hide resolved
src/trace.rs Outdated Show resolved Hide resolved
src/upgrade.rs Outdated Show resolved Hide resolved
…arly

Span macros in the `trace` module drop the `Span` early, making them
useless. The fix changes their implementation such that they return the
RAII guard, so the span is consumed and the guard is not dropped.
Remove an extraneous cfg attribute that was added erroneously in a
previous commit.
@RamziA961 RamziA961 marked this pull request as draft September 30, 2023 18:24
Remove `trace` documetationfrom `trace.rs`. Add unstable section to
`lib.rs` detailing unstable features. Add information about `tracing`
and `ffi` to unstable features section.
Change condition for when compile error is thrown in the `trace` module,
as the module is compiled even when tracing is disabled. Throw when
`unstable_hyper_tracing` is not specified while tracing flag is enabled.
Removed semi-colons after `Span.entered()` to ensure RAII guard is
returned.
Log errors as a field rather than part of a formatted string.
@RamziA961 RamziA961 marked this pull request as ready for review September 30, 2023 19:17
RamziA961 added a commit to RamziA961/hyper that referenced this pull request Oct 1, 2023
Similar to ffi, the feature job will skip tracing as it is unstable and
requires a cfg flag to compile without an error. Once hyperium#3326 is merged,
subsequent PRs will fail the feature CI job without this change.
@RamziA961 RamziA961 requested review from seanmonstar and hawkw October 2, 2023 16:17
@seanmonstar
Copy link
Member

All the things look good to me, thanks! Good job! You could include the change to the CI features check in this PR, so that it merges with the relevant code. Also, looks like there's merge conflicts.

Similar to ffi, the feature job will skip tracing as it is unstable and
requires a cfg flag to compile without an error. Once hyperium#3326 is merged,
subsequent PRs will fail the feature CI job without this change.
@RamziA961
Copy link
Contributor Author

All the things look good to me, thanks! Good job! You could include the change to the CI features check in this PR, so that it merges with the relevant code. Also, looks like there's merge conflicts.

Thank you. I moved the CI change here. It should be good to go.

@seanmonstar seanmonstar merged commit da3fc76 into hyperium:master Oct 2, 2023
18 checks passed
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
…erium#3326)

This change allow users to opt out of tracing via the `tracing` crate
by adding tracing as an optional feature. This change is part of the
effort, outlined in hyperium#2874, to reach hyper 1.0.

- Introduce several macros that act as wrappers around the `tracing`
module's macros to allow for more concise syntax for conditional
compilation of `tracing` macro calls. As `tracing` is unstable,
the new `trace` module will facilitate transitioning `tracing`
to an optional feature, as outlined in hyperium#2874 and hyperium#3326.
- Refactor code to utilize tracing macros from the new `trace` module.
- Add a `rustc` configuration flag (`hyper_unstable_tracing`) that must be passed
to the compiler when using the `tracing` feature. Throw a compiler error if `tracing`
is enabled without the flag. Part of the effort to transtition `tracing` to an
unstable dependecy. See hyperium#3326 and hyperium#2874.
- Modify `package.metadata.docs.rs` in `Cargo.toml` to include `tracing`
to list of `features` and `--cfg hyper_unstable_tracing` to the list of
`rustdoc-args`.
- Add unstable section to `lib.rs` detailing unstable features. Add information about `tracing`
and `ffi` to unstable features section.
- Log errors as a field rather than part of a formatted string.

Closes hyperium#3319

BREAKING CHANGES: tracing is disabled by default and requires users to
  opt in to an unstable feature to revert to previous behavior.
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024
…erium#3326)

This change allow users to opt out of tracing via the `tracing` crate
by adding tracing as an optional feature. This change is part of the
effort, outlined in hyperium#2874, to reach hyper 1.0.

- Introduce several macros that act as wrappers around the `tracing`
module's macros to allow for more concise syntax for conditional
compilation of `tracing` macro calls. As `tracing` is unstable,
the new `trace` module will facilitate transitioning `tracing`
to an optional feature, as outlined in hyperium#2874 and hyperium#3326.
- Refactor code to utilize tracing macros from the new `trace` module.
- Add a `rustc` configuration flag (`hyper_unstable_tracing`) that must be passed
to the compiler when using the `tracing` feature. Throw a compiler error if `tracing`
is enabled without the flag. Part of the effort to transtition `tracing` to an
unstable dependecy. See hyperium#3326 and hyperium#2874.
- Modify `package.metadata.docs.rs` in `Cargo.toml` to include `tracing`
to list of `features` and `--cfg hyper_unstable_tracing` to the list of
`rustdoc-args`.
- Add unstable section to `lib.rs` detailing unstable features. Add information about `tracing`
and `ffi` to unstable features section.
- Log errors as a field rather than part of a formatted string.

Closes hyperium#3319

BREAKING CHANGES: tracing is disabled by default and requires users to
  opt in to an unstable feature to revert to previous behavior.

Signed-off-by: Sven Pfennig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make tracing an unstable feature
3 participants